Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

recombine: add timeout configuration option #325

Merged
merged 1 commit into from
Dec 15, 2021
Merged

recombine: add timeout configuration option #325

merged 1 commit into from
Dec 15, 2021

Conversation

pmalek-sumo
Copy link
Contributor

No description provided.

@pmalek-sumo pmalek-sumo requested a review from a team December 2, 2021 18:00
@pmalek-sumo pmalek-sumo mentioned this pull request Dec 2, 2021
Closed
@codecov
Copy link

codecov bot commented Dec 2, 2021 •
edited
Loading

Codecov Report

Merging #325 (a08dc30) into main (450cd5b) will increase coverage by 0.0%.
The diff coverage is 88.2%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #325   +/-   ##
=====================================
  Coverage   76.9%   77.0%           
=====================================
  Files         94      94           
  Lines       4410    4427   +17     
=====================================
+ Hits        3395    3411   +16     
  Misses       698     698           
- Partials     317     318    +1
Impacted Files Coverage Δ
...perator/builtin/transformer/recombine/recombine.go 74.3% <88.2%> (+3.2%) ⬆️

@pmalek-sumo pmalek-sumo force-pushed the add-timeout-to-recombine-operator branch from aded0ff to 90f8fb7 Compare December 2, 2021 18:16
@pmalek-sumo
Copy link
Contributor Author

Performance-wise this doesn't show any signs of a regression:

go test -v -count 1 -run ^$ -bench Benchmark -benchmem -count 4 ./... # with patch
goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/recombine
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkRecombine
BenchmarkRecombine-16             419072              2894 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             395395              2849 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             410451              2874 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             436786              2874 ns/op             280 B/op         10 allocs/op
PASS
ok      github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/recombine   6.057s

go test -v -count 1 -run ^$ -bench Benchmark -benchmem -count 4 ./... # without patch
goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/recombine
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkRecombine
BenchmarkRecombine-16             423274              2872 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             434774              2834 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             432498              2845 ns/op             280 B/op         10 allocs/op
BenchmarkRecombine-16             437214              2847 ns/op             280 B/op         10 allocs/op
PASS
ok      github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/transformer/recombine   6.223s

djaglowski
djaglowski reviewed Dec 3, 2021
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmalek-sumo This looks good to me except I wonder if we should name this parameter force_flush_period, since it is basically the same as is done in file_input. What do you think?

operator/builtin/transformer/recombine/recombine.go Outdated Show resolved Hide resolved
@pmalek-sumo pmalek-sumo force-pushed the add-timeout-to-recombine-operator branch from 90f8fb7 to 961d6d9 Compare December 9, 2021 09:06
@djaglowski
Copy link
Member

@pmalek-sumo This looks good to me except I wonder if we should name this parameter force_flush_period, since it is basically the same as is done in file_input. What do you think?

@pmalek-sumo What do you think about this suggestion?

@pmalek-sumo
Copy link
Contributor Author

@pmalek-sumo This looks good to me except I wonder if we should name this parameter force_flush_period, since it is basically the same as is done in file_input. What do you think?

@pmalek-sumo What do you think about this suggestion?

I've missed that somehow. Seems reasonable. Let me rename this

@pmalek-sumo pmalek-sumo force-pushed the add-timeout-to-recombine-operator branch from 961d6d9 to cc938e4 Compare December 15, 2021 13:57
@pmalek-sumo
Copy link
Contributor Author

@pmalek-sumo This looks good to me except I wonder if we should name this parameter force_flush_period, since it is basically the same as is done in file_input. What do you think?

@pmalek-sumo What do you think about this suggestion?

Done, PTAL @djaglowski

@pmalek-sumo pmalek-sumo force-pushed the add-timeout-to-recombine-operator branch from cc938e4 to a08dc30 Compare December 15, 2021 13:58
@djaglowski djaglowski merged commit 7b3dec9 into open-telemetry:main Dec 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rockb1017 rockb1017 rockb1017 approved these changes

@djaglowski djaglowski djaglowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmalek-sumo @djaglowski @rockb1017 @pmalek